🐛 [Shopify] Defer plugin init until a checkout page view, fixing double session IDs - #4981
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 9cfaccb | Docs | View more details | Give us feedback! |
Bundles Sizes Evolution
|
30045a3 to
4d662d6
Compare
There was a problem hiding this comment.
💬 suggestion: If we want to decouple the logic for the onInit and onStart hooks, I would create two separate functions. We could keep it relatively simple:
/**
* Calls each plugin's `onInit`, and returns whether the initialization should go on: `false` if any
* plugin aborts it. Stays synchronous as long as no plugin returns a thenable.
*/
export function callPluginsOnInit(
plugins: RumPlugin[] | undefined,
parameter: { initConfiguration: RumInitConfiguration; publicApi: RumPublicApi }
): boolean | Promise<boolean> {
const results = (plugins ?? []).map((plugin) => plugin.onInit?.(parameter))
if (results.some(isThenable)) {
return Promise.all(results.map((result) => Promise.resolve(result))).then(
(resolvedResults) => !resolvedResults.includes(false)
)
}
return !results.includes(false)
}
export function callPluginsOnRumStart(plugins: RumPlugin[] | undefined, options: OnRumStartOptions): void {
for (const plugin of plugins ?? []) {
plugin.onRumStart?.(options)
}
}
| waitForThenable(Promise.resolve(result), DEFAULT_ON_INIT_TIMEOUT).catch((reason) => { | ||
| if (isTimeoutError(reason)) { | ||
| throw new Error(`Plugin ${plugins[index].name} onInit() timed out after ${DEFAULT_ON_INIT_TIMEOUT}ms`) | ||
| } | ||
| throw reason |
There was a problem hiding this comment.
Suggestion: keep things simple, don't handle timeouts.
There was a problem hiding this comment.
Do you mean just ignore them completely?
My motivation to handle it is to show some meaningful error message to the plugins consumers - so they will know which plugin has failed to init and why instead of generic Timeout error message.
Also I wound't say it adds a lot of complexity to ours code - just another .catch block, but let me know if you disagree
There was a problem hiding this comment.
Yes I would ignore it completely. If you really want a timeout, move it to the salesforce plugin.
The reason I am a bit reluctant is that is the main bundle size impact, with 0 benefit for the vast majority of usages.
| if (isBindingsInstalled) { | ||
| return | ||
| } |
There was a problem hiding this comment.
can you unsubscribe instead? This isBindingInstalled makes things more complex than necessary
There was a problem hiding this comment.
No, there is no option to unsubscribe from shopify events unfortunately :(
Here is official API reference for analytics.subscribe method and it's returning a Promise<undefined>, not an unsubscribe function or anything I can use later to detach the listener: https://shopify.dev/docs/api/web-pixels-api/standard-api/analytics
There was a problem hiding this comment.
Ok! Then nitpick: I would introduce a function like waitFirstPageViewedEvent(analytics) to isolate the subscription logic. You could even have an async onInit function:
async onInit({ initConfiguration, publicApi }) {
const analytics = configuration.shopifyAnalytics
if (!analytics) {
return false
}
const event = await waitFirstPageViewedEvent(analytics)
if (!isCheckoutPage(event)) {
return false
}
...
}
| @@ -90,6 +90,8 @@ describe('initShopifyBindings', () => { | |||
There was a problem hiding this comment.
I know this is not related exactly to this PR but while reviewing it I saw that maybe here we could use a loop like we do in other tests. Makes it more readable IMO.
it('starts a view on /checkout, /checkouts/*, and locale-prefixed checkout paths', () => {
const urls = [
'https://shop.example/checkout',
'https://shop.example/checkouts/abc123',
'https://shop.example/en-us/checkout',
]
for (const url of urls) {
expect(emitPageViewed(url)).toHaveBeenCalledTimes(1)
}
})
it('does not start a view on storefront, /orders/*, Customer Account pages, or an undefined url', () => {
const urls = [
'https://shop.example/products/foo',
'https://shop.example/orders/abc123',
'https://shop.example/account/orders',
undefined,
]
for (const url of urls) {
expect(emitPageViewed(url)).not.toHaveBeenCalled()
}
})
})
| // @ts-expect-error - shopifyAnalytics is required | ||
| const result = shopifyPlugin({}).onInit!({ initConfiguration, publicApi }) |
There was a problem hiding this comment.
What do you think of:
const result = shopifyPlugin({ shopifyAnalytics: undefined as unknown as ShopifyAnalyticsApi }).onInit!({ initConfiguration, publicApi })
That way we avoid the eslint ignore.
| function createFakeAnalytics() { | ||
| const subscribers = new Map<string, (event: ShopifyPixelEvent) => void>() | ||
| const analytics: ShopifyAnalyticsApi = { | ||
| subscribe: jasmine.createSpy('subscribe').and.callFake((eventName: string, callback) => { | ||
| subscribers.set(eventName, callback) | ||
| }), | ||
| } | ||
| return { | ||
| analytics, | ||
| emit: (eventName: string, event: ShopifyPixelEvent) => subscribers.get(eventName)?.(event), | ||
| } | ||
| } | ||
|
|
||
| function pageViewedEvent(url: string | undefined): ShopifyPixelEvent { | ||
| return { | ||
| name: 'page_viewed', | ||
| id: '1', | ||
| timestamp: '2026-07-06T00:00:00Z', | ||
| context: { document: { location: { href: url } } }, | ||
| } | ||
| } |
There was a problem hiding this comment.
createFakeAnalytics and pageViewedEvent are defined in the 3 spec files under domain. What about moving it into a src/test/mockShopifyAnalytics file where we export them?
| const results = plugins.map((plugin) => plugin.onInit?.(parameter)) | ||
|
|
||
| if (results.some(isThenable)) { | ||
| return Promise.all(results.map((result) => Promise.resolve(result))).then((results) => !results.includes(false)) |
There was a problem hiding this comment.
nitpick:
| return Promise.all(results.map((result) => Promise.resolve(result))).then((results) => !results.includes(false)) | |
| return Promise.all(results).then((results) => !results.includes(false)) |
No need to wrap into promises
4 participants
Motivation
Shopify Custom Pixel sandboxes were creating two RUM session IDs on the same page. The storefront's
Theme Liquid snippet already runs a
DD_RUMinstance for every page, while the Custom Pixel'sshopifyPluginunconditionally ran its owninit()side effects (patching sandboxed iframe APIs,wiring bindings, forcing
trackViewsManually, etc.) as soon asonInitfired — with no way to knowyet whether the page was actually a checkout page. See RFC: Preventing two SDK instances from
running at the same time
(RUM-18173).
Changes
RumPlugin.onInitcontract (packages/browser-rum-core/src/domain/plugins.ts,preStartRum.ts) soonInitmay returnfalseto abort SDK init, or aPromise<false | void>todefer it.
callPluginsMethod/runOnInitPluginsnow run plugins'onInitin order, stayingsynchronous until a plugin returns a thenable, and time out a pending
onInitafter 3s (surfacingan error rather than hanging init forever).
shopifyPlugin.onInitnow returns aPromisethat waits for the sandbox's firstpage_viewedevent and only proceeds (patches iframe APIs, wires bindings, forces sandbox-specific config) once
that event's URL matches a checkout path — so a Custom Pixel loaded on a non-checkout page no longer
spins up a second RUM instance.
initShopifyBindings'sclicked/ui_extension_erroredhandlers arenow gated the same way, via the shared
isCheckoutPagepredicate.makeShopifyRumPublicApi()init()-wrapping approach with the plugin-basedshopifyPlugin, now exposed asDD_RUM.shopifyPlugin(...)(see updatedpackages/browser-rum-shopify/README.md).Test instructions
yarn test:unit --spec packages/browser-rum-core/src/domain/plugins.spec.ts --spec packages/browser-rum-core/src/boot/preStartRum.spec.ts --spec packages/browser-core/src/tools/thenable.spec.ts --spec "packages/browser-rum-shopify/**/*.spec.ts"Checklist